Skip to content

[oauth2] Add isClosed and exception handling for closed state to oaut… #2089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[oauth2] Add isClosed and exception handling for closed state to oaut… #2089

wants to merge 1 commit into from

Conversation

illaz
Copy link

@illaz illaz commented May 8, 2025

Until now there is no way to detect whether an oauth2.Client is closed or not. This PR changes that.

There is now an isClosed field on the oauth2.Client.

A request on a closed client no longer leads to a 'Null check operator used on a null value' error, but instead an http.ClientException is thrown with a meaningful error message.

  • [x ] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@illaz illaz requested a review from a team as a code owner May 8, 2025 11:39
@sigurdm
Copy link
Contributor

sigurdm commented May 8, 2025

Should we just not set httpClient to null in close()? In that way we automatically inherit the close() behavior of the wrapped client...

@illaz
Copy link
Author

illaz commented May 9, 2025

Should we just not set httpClient to null in close()? In that way we automatically inherit the close() behavior of the wrapped client...

Would also be fine for me. This would at least avoid the 'Null check operator used on a null value' error.
I can also implement the isClosed behavior in my own code. It does not have to be included in this PR.

Is it okay to make a fixup commit here? Or should I just amend my existing commit?

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2025 •
edited
Loading

Package publishing

Package Version Status Publish tag (post-merge)
package:benchmark_harness 2.4.0-wip WIP (no publish necessary)
package:boolean_selector 2.1.2 already published at pub.dev
package:browser_launcher 1.1.3 already published at pub.dev
package:cli_config 0.2.1-wip WIP (no publish necessary)
package:cli_util 0.5.0-wip WIP (no publish necessary)
package:clock 1.1.3-wip WIP (no publish necessary)
package:code_builder 4.11.0 already published at pub.dev
package:coverage 1.15.0 already published at pub.dev
package:csslib 1.0.2 already published at pub.dev
package:extension_discovery 2.1.0 already published at pub.dev
package:file 7.0.2-wip WIP (no publish necessary)
package:file_testing 3.1.0-wip WIP (no publish necessary)
package:glob 2.1.3 already published at pub.dev
package:graphs 2.3.3-wip WIP (no publish necessary)
package:html 0.15.6 already published at pub.dev
package:io 1.1.0-wip WIP (no publish necessary)
package:json_rpc_2 4.0.0 already published at pub.dev
package:markdown 7.3.1-wip WIP (no publish necessary)
package:mime 2.0.0 already published at pub.dev
package:oauth2 2.0.4-wip WIP (no publish necessary)
package:package_config 2.3.0-wip WIP (no publish necessary)
package:pool 1.5.2 already published at pub.dev
package:process 5.0.5 already published at pub.dev
package:pub_semver 2.2.0 already published at pub.dev
package:pubspec_parse 1.5.0 already published at pub.dev
package:source_map_stack_trace 2.1.3-wip WIP (no publish necessary)
package:source_maps 0.10.14-wip WIP (no publish necessary)
package:source_span 1.10.1 already published at pub.dev
package:sse 4.1.8 already published at pub.dev
package:stack_trace 1.12.1 already published at pub.dev
package:stream_channel 2.1.4 already published at pub.dev
package:stream_transform 2.1.2-wip WIP (no publish necessary)
package:string_scanner 1.4.1 already published at pub.dev
package:term_glyph 1.2.3-wip WIP (no publish necessary)
package:test_reflective_loader 0.4.0 already published at pub.dev
package:timing 1.0.2 already published at pub.dev
package:unified_analytics 8.0.6 ready to publish unified_analytics-v8.0.6
package:watcher 1.1.5-wip WIP (no publish necessary)
package:yaml 3.1.3 already published at pub.dev
package:yaml_edit 2.2.2 already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2025 •
edited
Loading

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
oauth2 None 2.0.3 2.0.4-wip 2.0.3 ✔️

This check can be disabled by tagging the PR with skip-breaking-check.

Changelog Entry ❗
Package Changed Files
package:oauth2 pkgs/oauth2/lib/src/client.dart

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

Coverage ✔️
File Coverage
pkgs/oauth2/lib/src/client.dart 💚 95 % ⬆️ 9 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

License Headers ✔️ 
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/bazel_worker/benchmark/benchmark.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/coverage/lib/src/coverage_options.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/html/test/trie_test.dart
pkgs/html/tool/generate_trie.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

This check can be disabled by tagging the PR with skip-license-check.

@mosuem
Copy link
Member

mosuem commented Oct 7, 2025

@sigurdm friendly ping!

sigurdm
sigurdm reviewed Oct 7, 2025
View reviewed changes
sigurdm
sigurdm reviewed Oct 7, 2025
View reviewed changes
sigurdm
sigurdm reviewed Oct 7, 2025
View reviewed changes
Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this again see http.Client doesn't have a isClosed() method. That suggests to me the OauthClient also shouldn't. Usually you should know if the client has been closed or not from the state of your code.

I think it is valuable to remove the "_httpClient = null" from the close() method. That should avoid the null pointer reference.

And yes, it is very fine to just push more commits to the same PR.

@illaz illaz force-pushed the bugfix/add-is-closed branch from 56aac36 to 83b8e6f Compare October 8, 2025 18:35
@illaz
Copy link
Author

illaz commented Oct 8, 2025

@sigurdm Thank you for the suggestions. I amended the commit and also adapted the commit message. The branch name doesn't fit 100%, but if that matters, I could open a new PR.

@sigurdm
Copy link
Contributor

sigurdm commented Oct 9, 2025

The branch name has no effect. Don't mind that :)

@sigurdm
Copy link
Contributor

sigurdm commented Oct 9, 2025

I see there is a lint triggering. Could you fix that?

@illaz illaz force-pushed the bugfix/add-is-closed branch from 83b8e6f to 19e3781 Compare October 9, 2025 12:08
@illaz
Copy link
Author

illaz commented Oct 9, 2025

I see there is a lint triggering. Could you fix that?

Done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sigurdm sigurdm sigurdm approved these changes

Assignees
No one assigned
Labels
package:oauth2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@illaz @sigurdm @mosuem